-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security solution] Guided onboarding, alerts & cases #143598
[Security solution] Guided onboarding, alerts & cases #143598
Conversation
@@ -83,12 +82,6 @@ export const GlobalHeader = React.memo( | |||
}; | |||
}, [portalNode, setHeaderActionMenu, theme.theme$]); | |||
|
|||
const { isTourShown, endTour } = useTourContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this was here, but we are changing the tour entirely so i think safe to remove?
Yes. Happy path means the user clicked through exactly as we wanted them to like a good little user. I need to go back and handle the cases when the user clicks out of the tour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
@elasticmachine merge upstream |
x-pack/plugins/security_solution/public/common/components/event_details/event_details.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this, @stephmilovic!
I tested the happy path locally and it worked as expected 👍
x-pack/plugins/security_solution/public/timelines/components/timeline/body/actions/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour_step.tsx
Outdated
Show resolved
Hide resolved
const [loading, setLoading] = useState(false); | ||
|
||
// loading = false initial state causes flashes of empty tables | ||
const [loading, setLoading] = useState(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does purely cosmetic onboarding elements being added cause tables to flash empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was already flashing, it just became obvious with the addition of the EuiTourStep. if it starts as not loading, then loading, the element is there briefly and then disappears. EuiTourStep uses EuiPortal to mount. So it mounts to the flyout when it shows briefly, hides when the flyout flashes blank and goes into loading state, and comes back when flyout is actually available
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
miscellaneous assets size
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
* main: (24 commits) [Files] Add file upload to file picker (elastic#143969) [Security solution] Guided onboarding, alerts & cases (elastic#143598) [APM] Critical path for a single trace (elastic#143735) skip failing test suite (elastic#143933) [Fleet] Update GH Projects automation (elastic#144123) [APM] Performance fix for 'cardinality' telemetry task (elastic#144061) [Enterprise Search] Attach ML Inference Pipeline - Pipeline re-use (elastic#143979) [8.5][DOCS] Add support for differential logs (elastic#143242) Bump nwsapi from v2.2.0 to v2.2.2 (elastic#144001) [APM] Add waterfall to dependency operations (elastic#143257) [Shared UX] Add deprecation message to kibana react Markdown (elastic#143766) [Security Solution][Endpoint] Adds RBAC API checks for Blocklist (elastic#144047) Improve `needs-team` auto labeling regex (elastic#143787) [Reporting/CSV Export] _id field can not be formatted (elastic#143807) Adds SavedObjectsWarning to analytics results pages. (elastic#144109) Bump chromedriver to 107 (elastic#144073) Update cypress (elastic#143755) [Maps] nest security layers in layer group (elastic#144055) [Lens][Agg based Heatmap] Navigate to Lens Agg based Heatmap. (elastic#143820) Added support of saved search (elastic#144095) ...
## Summary Fixes #181190 Relevant PRs: #143598 | #163739 ### Steps to Verify: 1. Entering [these](https://p.elstc.co/paste/sEmk++Tb#mjwuX7IN8hIN+kOy5gdtfweQNi9sUl+4lVRAewc6hR+) in your kibana.dev.yml 2. Execute this command to set the Guided onboarding steps to alertsCases ``` curl --location --request PUT 'http://localhost:5601/internal/guided_onboarding/state' \ --header 'kbn-xsrf: cypress-creds' \ --header 'x-elastic-internal-origin: security-solution' \ --header 'Content-Type: application/json' \ --header 'Authorization: Basic {YOUR_AUTH_TOKEN}' \. // If you are using Postman just fill in the Auth tab --data '{ "status": "in_progress", "guide": { "isActive": true, "status": "in_progress", "steps": [ { "id": "add_data", "status": "complete" }, { "id": "rules", "status": "complete" }, { "id": "alertsCases", "status": "active" } ], "guideId": "siem" } }' ``` 3. Make sure you have alerts available. 4. To test the old flyout with Guided onboarding tour, please go to Stack Management > Advanced settings > Expandable flyout **OFF** ### It compatible with the new expandable flyout: 1. It shows `expandable flyout tour` when the guided onboarding tour is **not enabled**. 3. The first two steps should be `hidden` when the `left` expandable is opened. 5. Most of the guided onboarding tour steps should be hidden when `Add to new case` flyout or `Add to existing case` modal opened. 6. Once the test case is created, the `insight section` and `correlation tab` should be opened automatically to fetch cases. 7. `expandable flyout tour` should be visible again after the guided onboarding tour is finished. https://github.com/elastic/kibana/assets/6295984/b19bfce9-ec02-4291-b616-e24d3e984a03 ### It compatible with the old flyout: https://github.com/elastic/kibana/assets/6295984/b10b8bdf-e159-4663-b455-1f4541358a11 ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Sergi Massaneda <sergi.massaneda@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Dear reviewer, please read this whole description before reviewing the code...
ICYMI
Security currently have a product tour that needs to be removed. There is a large effort in the Onboarding team to bring a unified flow to onboard users to each solution in Kibana. For 8.6 the Security team needs to replace our old product tour with the new
guidedOnboarding
plugin framework. There are 3 steps for Security to implement: 1. Add Data 2. Turn on Rules 3. Alerts and CasesSummary
guidedOnboarding
plugin dependencySecurityStepId
SecurityStepId.alertsCases
Still to do after merging
It is important to merge this work so that @xcrzx can use the
guidedOnboarding
plugin for his steps. Therefore this PR only implements the happy path. Still to do:I know there are some weird cases like this, please ignore:
To test
This is only step 3 of the tour and the previous have not been done yet, so we need to manually put Kibana in the correct state to trigger step 3
xpack.securitySolution.enableExperimental: ['guidedOnboarding']
yarn start --run-examples
/app/guidedOnboardingExample
security
and Step ID toalertsCases
Brain dump
This work required some creativity for reasons. Allow me to explain some weirdness
The
EuiTourStep
component needs an anchor to attach on in the DOM. This can be defined in 2 ways:It was important that the
EuiTourStep
anchor is in the DOM when the tour step becomes active. Additionally, when the anchor leaves the DOM, we needEuiTourStep
to leave the DOM as well.How to use components (for D&R step engineers)
../public/common/components/guided_onboarding_tour/tour_config.ts
in thesecurityTourConfig
constGuidedOnboardingTourStep
component at the location of the anchor. As stated in the Brain Dump section, there are two ways to define the anchor. I will explain examples of both methods:Method 1 - as children. Looking at step 1 of the
SecurityStepId.alertsCases
tour. In thealertsCasesConfig
you can see the config for this step looks like:Notice that no anchor prop is defined in the step 1 config.
As you can see pictured below, the tour step anchor is the Rule name of the first alert.
The component for this anchor is
RenderCellValue
which returnsDefaultCellRenderer
. We wrapDefaultCellRenderer
withGuidedOnboardingTourStep
, passingstep={1} stepId={SecurityStepId.alertsCases}
to indicate the step. Since there are many other iterations of this component on the page, we also need to pass theisTourAnchor
property to determine which of these components should be the anchor. In the code, this looks something like:Method 2 - as anchor props. Looking at step 5 of the
SecurityStepId.alertsCases
tour. In thealertsCasesConfig
you can see the config for this step looks like:Notice that the anchor prop is defined as
[data-test-subj="create-case-flyout"]
in the step 5 config. There is also ahideNextButton
boolean utilized here.As you can see pictured below, the tour step anchor is the create case flyout and the next button is hidden.
HINT: @cnasikas and @jonathan-buttner @elastic/response-ops-cases this is where your part of the review comes in
Since cases is its own plugin and we are using a method to generate the flyout, we cannot wrap the flyout as children of the
GuidedOnboardingTourStep
. We do however need theEuiTourStep
component to mount in the same location as the anchor. Therefore, I had to pass a new optional property to the case component calledheaderContent
that simply accepts and rendersReact.ReactNode
at the top of the flyout. In the code, this looks something like:The
useTourContext
is used within anchor components, returning the state of the security tourWhen the tour step does not have a next button, the anchor component will need to call
incrementStep
after an action is taken. For example, inSecurityStepId.alertsCases
step 4, the user needs to click the "Add to case" button to advance the tour.So we utilize the
useTourContext
to do the following check and increment the step inhandleAddToNewCaseClick
:In
SecurityStepId.alertsCases
step 5, the user needs to fill out the form and hit the "Create case" button in order to end thealertsCases
portion the tour, so with theafterCaseCreated
method we callendTourStep(SecurityStepId.alertsCases)
.